Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contract artifact propagation tech debt #408

Merged
merged 10 commits into from
May 4, 2023

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Apr 28, 2023

Description

Fixes #209
Fixes #407

Note 1: I enabled l1-contract tests in CI because I needed to have the image available later on in CI pipeline to generate the TS artifacts.

Note 2: I fully disabled the l2-block-publisher test because it will need a larger refactoring after removing ethereum.js and it's not yet clear what we'll want to do with the test (see comment).

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I wouldn't merge until we get buy in that we're going all-in with viem over ethereum-js, since this removes support for it.

@benesjan benesjan marked this pull request as draft April 28, 2023 17:26
@benesjan benesjan force-pushed the janb/contract-artifacts-tech-debt branch 9 times, most recently from 8671564 to 91c9a2b Compare May 3, 2023 07:52
@benesjan benesjan requested a review from spalladino May 3, 2023 11:52
Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left a few comments.

build_manifest.json Outdated Show resolved Hide resolved
build_manifest.json Outdated Show resolved Hide resolved
yarn-project/foundation/package.json Show resolved Hide resolved
yarn-project/l1-artifacts/tsconfig.json Show resolved Hide resolved
yarn-project/types/src/initial_l2_block_num.ts Outdated Show resolved Hide resolved
@benesjan benesjan marked this pull request as ready for review May 3, 2023 15:01
@benesjan benesjan force-pushed the janb/contract-artifacts-tech-debt branch from 67070ce to 8c435f3 Compare May 3, 2023 15:27
@benesjan benesjan force-pushed the janb/contract-artifacts-tech-debt branch from 97672c2 to 01942eb Compare May 4, 2023 08:29
@benesjan benesjan merged commit 4f144b7 into master May 4, 2023
@benesjan benesjan deleted the janb/contract-artifacts-tech-debt branch May 4, 2023 09:11
ludamad added a commit that referenced this pull request Jul 14, 2023
* More generators for aztec3.

* update js vk (because we now use UP for merkle hashing)

* Helpers for ECDSA in A3 (#364)

* Add `stdlib_keccak` in cmake.

Correct an assertion in `to_byte_array` in bigfield.

* Add `random_element` to affine element.

* negate y conditionally.

* Change pedersen hash c_bind to use `pedersen_hash::lookup`.

* c_binds and other ECDSA related fixes (#407)

* Add v to stdlib ecdsa.

* create an engine if its empty.

* Add ecdsa c_bind.

* print v as a uint32.

* Add secp256k1 cbind.

add c_bind.hpp

Change hpp to h.

remove hpp.

* Add ecdsa in cmakelists.

remove stdlib_ecdsa from build.

* hack: (aztec3) introduce barretenberg crypto generator parameters hack (#408)

* hack: introduce BARRETENBERG_CRYPTO_GENERATOR_PARAMETERS_HACK

* doc: concise

* chore: align BARRETENBERG_CRYPTO_GENERATOR_PARAMETERS_HACK usage

* CI to test aztec circuits with current commit of bberg

* build manifest

* chore: align BARRETENBERG_CRYPTO_GENERATOR_PARAMETERS_HACK usage (#411)

* try other branch of aztec packages

* ci rename script

* Update join_split test

* bump aztec version and merge in aztec3-temporary fixes

* aztec commit switched to branch

* bump aztec commit and document

* typo README.md

* Update README.md

---------

Co-authored-by: Suyash Bagad <[email protected]>
Co-authored-by: Suyash Bagad <[email protected]>
Co-authored-by: ludamad <[email protected]>
Co-authored-by: ludamad <[email protected]>
codygunton pushed a commit that referenced this pull request Jan 23, 2024
* More generators for aztec3.

* update js vk (because we now use UP for merkle hashing)

* Helpers for ECDSA in A3 (#364)

* Add `stdlib_keccak` in cmake.

Correct an assertion in `to_byte_array` in bigfield.

* Add `random_element` to affine element.

* negate y conditionally.

* Change pedersen hash c_bind to use `pedersen_hash::lookup`.

* c_binds and other ECDSA related fixes (#407)

* Add v to stdlib ecdsa.

* create an engine if its empty.

* Add ecdsa c_bind.

* print v as a uint32.

* Add secp256k1 cbind.

add c_bind.hpp

Change hpp to h.

remove hpp.

* Add ecdsa in cmakelists.

remove stdlib_ecdsa from build.

* hack: (aztec3) introduce barretenberg crypto generator parameters hack (#408)

* hack: introduce BARRETENBERG_CRYPTO_GENERATOR_PARAMETERS_HACK

* doc: concise

* chore: align BARRETENBERG_CRYPTO_GENERATOR_PARAMETERS_HACK usage

* CI to test aztec circuits with current commit of bberg

* build manifest

* chore: align BARRETENBERG_CRYPTO_GENERATOR_PARAMETERS_HACK usage (#411)

* try other branch of aztec packages

* ci rename script

* Update join_split test

* bump aztec version and merge in aztec3-temporary fixes

* aztec commit switched to branch

* bump aztec commit and document

* typo README.md

* Update README.md

---------

Co-authored-by: Suyash Bagad <[email protected]>
Co-authored-by: Suyash Bagad <[email protected]>
Co-authored-by: ludamad <[email protected]>
Co-authored-by: ludamad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bootstrapping /l1-contracts doesn't make new artifacts available in TS codebase Settle on a web3 library
2 participants